New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support Map and Arrays of Maps in BQ for StorageWrites for Beam Rows #22179

Closed

prodriguezdefino wants to merge 11 commits into apache:master from prodriguezdefino:support_maps_plus_arrayofmap_bq_storagewrites

Contributor

prodriguezdefino commented Jul 7, 2022 •

edited

Loading

Currently BigQuery table schema utility, and the implementation for StorageWrites for Beam Rows, does not support sending records with Maps as part of their schema.

This PR adds that functionality transforming the Map into a Message type which contains two fields key and value respecting the types coming from upstream while mimicking the behavior when using TableRows to the BigQueryIO PTransform.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.


          adding support for maps and array of maps to BQ when using StorageWrites

307ce71

github-actions bot added gcp io java labels

Contributor Author

prodriguezdefino commented Jul 9, 2022

Run Java PreCommit

Contributor

github-actions bot commented Sep 8, 2022

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions bot added the stale label

prodriguezdefino and others added 3 commits

October 12, 2022 09:36


          Merge branch 'master' into support_maps_plus_arrayofmap_bq_storagewrites

00ecf6b


          adding tests for new functionality

10aa27a


          spotless apply

7c41bee

Contributor Author

prodriguezdefino commented Oct 12, 2022

fixes #23618

prodriguezdefino marked this pull request as ready for review

October 12, 2022 22:25

prodriguezdefino changed the title ~~Support Map and Arrays of Maps in BQ for StorageWrites~~ Support Map and Arrays of Maps in BQ for StorageWrites for Beam Rows


          fixes checkstyle error

c9e22d6

Contributor Author

prodriguezdefino commented Nov 21, 2022 •

edited

Loading

.remove-labels stale

Contributor

reuvenlax commented Nov 30, 2022

Can you rebase and resolve, since that file has since been refactored?

prodriguezdefino added 2 commits

November 30, 2022 15:27


          Merge branch 'master' into support_maps_plus_arrayofmap_bq_storagewrites

27683f8


          spotless apply

7cf633b

Contributor Author

prodriguezdefino commented Dec 1, 2022

Can you rebase and resolve, since that file has since been refactored?

done

reuvenlax reviewed

View reviewed changes

...oud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java Outdated

@@ @@ -229,6 +243,8 @@ private static Object messageValueFromRowValue( @@
                   if (value == null) {
                     if (fieldDescriptor.isOptional()) {
                       return null;
+                    } else if (fieldDescriptor.isRepeated()) {
+                      return Lists.newArrayList();

Contributor

reuvenlax Dec 1, 2022

Collections.emptyList()

Contributor Author

prodriguezdefino Dec 1, 2022

Done

...oud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java Outdated

+                          arrayElementType.getTypeName().isCollectionType()
+                                  || arrayElementType.getTypeName().isMapType()
+                              ? true
+                              : false;

Contributor

reuvenlax Dec 1, 2022

remove redundant ternary operator. Also no need for Boolean

Contributor Author

prodriguezdefino Dec 1, 2022

Done (what was I thinking)

...oud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java

+                      if (shouldFlatMap) {
+                        valueStream = valueStream.flatMap(vs -> ((List) vs).stream());
+                      }

Contributor

reuvenlax Dec 1, 2022

explain why flatMap is correct here?

Contributor Author

prodriguezdefino Dec 1, 2022 •

edited

Loading

Because BQ does not support arrays of maps, this code is making the decision of flattening those structures (the if condition is computed based on that particular scenario).

[
map1 [k1,v2] [k2,v2] ,
map2 [k3,v3],
map3 [k4,v4] [k5,v5],
]

------------------------- to
[
record {key:k1, value:v1},
record {key:k2, value:v2},
record {key:k3, value:v3},
record {key:k4, value:v4},
record {key:k5, value:v5}
]

It respects the order in the array and the inherent order of iteration in the maps, but it won't check for repeated keys across the maps in the original array.

...oud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java Outdated

+                    FieldType keyFieldType,
+                    FieldType valueFieldType,
+                    Map.Entry<Object, Object> entryValue) {

Contributor

reuvenlax Dec 1, 2022

remove extra line

Contributor Author

prodriguezdefino Dec 1, 2022

Done

...oud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BeamRowToStorageApiProto.java

+                  FieldDescriptor keyFieldDescriptor =
+                      Preconditions.checkNotNull(descriptor.findFieldByName("key"));
+                  @Nullable Object key = toProtoValue(keyFieldDescriptor, keyFieldType, entryValue.getKey());
+                  if (key != null) {

Contributor

reuvenlax Dec 1, 2022

are null keys allowed?

Contributor Author

prodriguezdefino Dec 1, 2022

Good question, AFAICT the backing Map in the Row object is a HashMap (with expected size) so I would say that null keys are allowed in a map property for a Row object.
This code only ignores setting the value in the proto if null was the value present on the map's key.

prodriguezdefino added 2 commits

December 1, 2022 09:40


          addressing comments

5e450e9


          fixing spotless expected format

5f588e5

Contributor Author

prodriguezdefino commented Dec 1, 2022

Run Java PreCommit

Contributor Author

prodriguezdefino commented Dec 1, 2022

Run Java_GCP_IO_Direct PreCommit

Contributor Author

prodriguezdefino commented Dec 2, 2022

Is there a way to rerun Java Tests / Java Unit Tests (ubuntu-latest) (pull_request) those are failing because:

org.apache.beam.fn.harness.jmh.ProcessBundleBenchmarkTest > testStateWithoutCaching FAILED
[176](https://github.com/apache/beam/actions/runs/3595444602/jobs/6054940468#step:5:177)
    java.util.concurrent.ExecutionException at ProcessBundleBenchmarkTest.java:47
[177](https://github.com/apache/beam/actions/runs/3595444602/jobs/6054940468#step:5:178)
        Caused by: java.lang.RuntimeException at ProcessBundleBenchmark.java:175
[178](https://github.com/apache/beam/actions/runs/3595444602/jobs/6054940468#step:5:179)
            Caused by: java.lang.RuntimeException at UnboundedScheduledExecutorService.java:316
[179](https://github.com/apache/beam/actions/runs/3595444602/jobs/6054940468#step:5:180)
                Caused by: java.lang.InterruptedException at FutureTask.java:418

Not related with this changes, but not sure how to ensure this works without pushing more commits.

Contributor

reuvenlax commented Dec 2, 2022

Has this been tested e2e (with BigQuery)?

prodriguezdefino requested a review from reuvenlax

December 6, 2022 05:09


          Merge branch 'master' into support_maps_plus_arrayofmap_bq_storagewrites

e6d7d56

github-actions bot removed the stale label


          ran spotless apply

cda4069

Contributor Author

prodriguezdefino commented Jan 31, 2023

Has this been tested e2e (with BigQuery)?

yes.

map, job id: 2023-01-30_16_31_29-9471872084021240643

list/array of maps job id: 2023-01-30_16_32_57-10318556278839602747
schema on BQ:

Contributor

reuvenlax commented Jan 31, 2023

Run Java_GCP_IO_Direct PreCommit

1 similar comment

Contributor Author

prodriguezdefino commented Jan 31, 2023

Run Java_GCP_IO_Direct PreCommit

prodriguezdefino added a commit to prodriguezdefino/apache-beam-streaming-tests that referenced this pull request


          testing apache/beam#24274 and apache/beam#22179

9196d4a

prodriguezdefino mentioned this pull request

Changes after Avro related changes on BigQueryIO prodriguezdefino/apache-beam-streaming-tests#1

Merged

Contributor

github-actions bot commented Apr 1, 2023

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions bot added stale and removed stale labels

Contributor

github-actions bot commented Jun 8, 2023

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions bot added the stale label

Contributor

github-actions bot commented Jun 15, 2023

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot closed this

Contributor

JohnZZGithub commented Sep 20, 2024

Could we merge the PR? This is necessary for us to ingest data to BigQuery.

Contributor Author

prodriguezdefino commented Sep 20, 2024 via email

This PR has been closed for a while, I can take some time to migrate the changes to a new branch from master.

On Thu, Sep 19, 2024 at 6:03 PM zz ***@***.***> wrote: Could we merge the PR? This is necessary for us to ingest data to BigQuery. — Reply to this email directly, view it on GitHub <#22179 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2HMFYXRBLP2I6TEUYYPJTZXNX67AVCNFSM6AAAAABORALTZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRSGUYDSMJTGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

prodriguezdefino mentioned this pull request

Support Map in BQ for StorageWrites API for Beam Rows #32512

Merged

3 tasks

Contributor

JohnZZGithub commented Sep 20, 2024

This PR has been closed for a while, I can take some time to migrate the changes to a new branch from master.
…
On Thu, Sep 19, 2024 at 6:03 PM zz @.> wrote: Could we merge the PR? This is necessary for us to ingest data to BigQuery. — Reply to this email directly, view it on GitHub <#22179 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2HMFYXRBLP2I6TEUYYPJTZXNX67AVCNFSM6AAAAABORALTZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRSGUYDSMJTGQ . You are receiving this because you authored the thread.Message ID: @.>

Thanks a lot Pablo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gcp io java stale